Adding PyGILState_Check() in object_api<>::operator(). #2919
Conversation
|
Estimation of worst-case runtime overhead. Compiler: Debian clang version 11.0.1-2 Results with PR as-is (except with Results with this diff: diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index 32257074..c55d1007 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -1348,11 +1348,11 @@ unpacking_collector<policy> collect_arguments(Args &&...args) {
template <typename Derived>
template <return_value_policy policy, typename... Args>
object object_api<Derived>::operator()(Args &&...args) const {
-#if defined(NDEBUG) && PY_VERSION_HEX >= 0x03060000
+//#if !defined(NDEBUG) && PY_VERSION_HEX >= 0x03060000
if (!PyGILState_Check()) {
pybind11_fail("pybind11::object_api<>::operator() PyGILState_Check() failure.");
}
-#endif
+//#endif
return detail::collect_arguments<policy>(std::forward<Args>(args)...).call(derived().ptr());
} |
|
Overall, looks great to me! I think the usage of Though was kinda hard for me to initially interpret benchmark results as a frequency in GHz. |
Possibly, this explains why PyGILState_Check() cannot safely be used with Python 3.4 and 3.5: python/cpython#10267 (comment)
rwgk
force-pushed
the
master_py_gil_state_check
branch
from
491d670 to
ef5a014
Compare
|
Please do not remove the changelog entry section from the template, it gets automatically collected. Plus it is a nice one sentence summary of what you are doing. :) (I think I'll be adding this comment to my auto-replies, or adding a bot to comment this automatically...) |
tests/test_callbacks.py
Outdated
| @@ -146,3 +147,33 @@ def test_async_async_callbacks(): | |||
| t = Thread(target=test_async_callbacks) | |||
| t.start() | |||
| t.join() | |||
|
|
|||
|
|
|||
| def test_callback_num_times(capsys): | |||
There was a problem hiding this comment.
Eventually, we could require pytest-benchmark, or something similar. We could even do it manually; add a benchmark mark and if that mark is passed in, run the benchmark tests, and otherwise not run this or run this for a minimal number of times.
Actually, I don't really like a printout that always happens on all test runs. I rather think this should be opt in (via a mark).
There was a problem hiding this comment.
FWIW pytest always captures the output, and only reports on error, right?
Hence the recommendation for stuff like --capture=no here: https://github.com/pybind/pybind11/blob/f676782becc5bcd30c56b4da1ff1026db61bcce3/.github/CONTRIBUTING.md#configuration-options
So a defect in some contexts, but prolly not here? (i.e. it's probably good to let it run since it's short?)
There was a problem hiding this comment.
I removed capsys entirely. Simply running with -k test_callback_num_times -s is easy and best.
There was a problem hiding this comment.
Okay. I think we should probably add a mark for this eventually, but not for just one item yet. @EricCousineau-TRI I believe @rwgk disabled capturing explicitly so that this would print out before, but he's removed that now.
There was a problem hiding this comment.
Ah, derp 🤦 Thanks for explaining!
…ing `.format()`.
I overlooked this comment before, but I will leave this as calls / second, mainly because I don't want to spend the extra time updating the comment with the results, but also because that seems a lot more intuitive to me, feeling like speaking for people who don't do benchmarks all the time (most). |
|
Please remember to add the changelog entry to the issue description! I can copy-and-paste the template back in. |
Done. Thanks @henryiii and @EricCousineau-TRI ! |
Keywords: callbacks, Python Global Interpreter Lock (GIL)
Problem addressed: GIL not held when C++ calls back into Python. This is a fairly common accident, easily overlooked, and notoriously difficult/time-consuming to diagnose.
This PR adds a safety guard in one strategic location, guarded by
NDEBUG.NDEBUGis not defined the guard ensures that the Python GIL is held when C++ calls back into Python viaobject_api<>::operator()(e.g.py::function__call__). The worst-case runtime overhead (for a callback that returns immediately without doing any work) is ~10%. See detailed timings under Adding PyGILState_Check() in object_api<>::operator(). #2919 (comment). The typical runtime overhead (callback doing work) is probably much smaller.NDEBUGis defined, the guard is disabled, there is no protection, and zero runtime overhead.This PR was tested in the Google internal environment, which lead to the discovery of a few GIL-related issues and their cleanup. (The Google internal testing currently covers a couple hundred pybind11 extensions, including many imported or exported OSS packages.)
Suggested changelog entry: